Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add execution options for processor and prover #991

Merged
merged 3 commits into from
Jul 18, 2023

Conversation

Fumuran
Copy link
Contributor

@Fumuran Fumuran commented Jul 4, 2023

This PR adds implementation and usage for the ExecutionOptions struct, used to limit the maximum number of cycles and set their estimated number. CLI was changed to parse new parameters for the ExecutionOptions.
This is the preparatory PR, that is data in this struct is not used anywhere yet. The actual limitations will be implemented in the next PR.

@Fumuran Fumuran linked an issue Jul 4, 2023 that may be closed by this pull request
@Fumuran Fumuran force-pushed the andrew-processor-exec-options branch 3 times, most recently from c4e50f0 to bac3fb1 Compare July 5, 2023 22:14
@Fumuran Fumuran marked this pull request as ready for review July 6, 2023 20:40
@Fumuran Fumuran requested review from bobbinth and tohrnii July 6, 2023 23:03
Copy link
Contributor

@tohrnii tohrnii left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you. Looks good to me.

Copy link
Contributor

@bobbinth bobbinth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! Looks good! This is not a full review yet - but I left some comments which will probably affect quite a few files.

air/src/execution.rs Outdated Show resolved Hide resolved
air/src/execution.rs Outdated Show resolved Hide resolved
air/src/proof.rs Outdated Show resolved Hide resolved
air/src/proof.rs Outdated Show resolved Hide resolved
air/src/proof.rs Outdated Show resolved Hide resolved
air/src/proof.rs Outdated Show resolved Hide resolved
miden/Cargo.toml Outdated Show resolved Hide resolved
processor/src/lib.rs Outdated Show resolved Hide resolved
processor/src/lib.rs Outdated Show resolved Hide resolved
@Fumuran Fumuran force-pushed the andrew-processor-exec-options branch 4 times, most recently from 7a57b4f to e410488 Compare July 8, 2023 22:09
@Fumuran Fumuran requested a review from bobbinth July 8, 2023 22:44
@Fumuran Fumuran force-pushed the andrew-processor-exec-options branch from cc03814 to e7f1044 Compare July 14, 2023 21:49
Copy link
Contributor

@bobbinth bobbinth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! Looks good! I left small comments inline.

processor/README.md Outdated Show resolved Hide resolved
test-utils/Cargo.toml Outdated Show resolved Hide resolved
prover/src/lib.rs Outdated Show resolved Hide resolved
miden/README.md Outdated Show resolved Hide resolved
miden/README.md Outdated Show resolved Hide resolved
air/src/options.rs Show resolved Hide resolved
air/src/errors.rs Show resolved Hide resolved
air/src/errors.rs Outdated Show resolved Hide resolved
air/src/errors.rs Outdated Show resolved Hide resolved
miden/src/cli/prove.rs Outdated Show resolved Hide resolved
@Fumuran Fumuran requested a review from bobbinth July 18, 2023 11:47
Copy link
Contributor

@bobbinth bobbinth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Thank you! I left one small nit inline. Once this is resolved, we can merge.

Comment on lines 92 to 94
/// Sets [ExecutionOptions] for this [ProvingOptions].
/// This sets the maximum number of cycles a program is allowed to execute as well as
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: we should add a blank line between the intro sentence and the rest of the comment. Otherwise, doc formatter will not format them well.

@Fumuran Fumuran force-pushed the andrew-processor-exec-options branch from ff6d0dd to 2dca4b9 Compare July 18, 2023 19:15
@Fumuran Fumuran force-pushed the andrew-processor-exec-options branch from 2dca4b9 to c61da48 Compare July 18, 2023 19:17
@Fumuran Fumuran merged commit f2d9e8f into next Jul 18, 2023
15 checks passed
@Fumuran Fumuran deleted the andrew-processor-exec-options branch July 18, 2023 19:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants